Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Assignment from TensorMap wrapping a const type #94

Merged
merged 1 commit into from
May 30, 2020

Conversation

feltech
Copy link
Contributor

@feltech feltech commented May 30, 2020

This is more of a bug report/feature request than a real pull request, since I'm certain the solution (and possibly the problem description) here is incomplete at best. The additional unit test shows the situation :

        const T data[] = {1, 2, 3};
        TensorMap<const T, 3> mdata{data};
        Tensor<T, 3> tdata = mdata;

Prior to this patch, the above will not compile. That is, we cannot assign a TensorMap wrapping an array of const qualified elements to a Tensor containing non-const elements.

For background, I came across this issue when wanting to copy the (3-dimensional) positions of the 4 nodes in a tetrahedron into a 4x3 tensor. Those node positions are passed as const-qualified (non-Fastor) vectors, which I attempted to TensorMap then assign each in turn to a slice of my Tensor. But of course that didn't work.

This pull request does the very minimum to get the above situation working. I'm happy for you to take it over or replace it, or I can do minor tweaks as requested. I started using Fastor only a couple of days ago, so I'm probably not the best person to be fiddling ;)

A brief summary of changes:

  • Use destination type (e.g. non-const) to construct SIMDVector in eval functions, otherwise we get "assignment of read-only location" errors.
  • Don't const-qualify destination tensor in assign, otherwise overload resolution of called trivial_assign fails.
  • Missing inline declaration for a maskstore overload causing "multiple definition of Fastor::maskstore" errors (this is unrelated to the issue of assigning const data, but caused compilation errors for me - should probably be a separate PR/issue).

…sor` storing non-`const`

* Add test that copy-assigns to a `Tensor` from a `TensorMap` wrapping an array of `const`-qualified data.
* Missing inline declaration for a `maskstore` overload causing "multiple definition of `Fastor::maskstore`" errors.
* Use destination type (i.e. non-const) to construct `SIMDVector` in `eval` functions, otherwise we get "assignment of read-only location" errors.
* Don't `const`-qualify destination tensor in `assign`, otherwise overload resolution of called `trivial_assign` fails.
@romeric
Copy link
Owner

romeric commented May 30, 2020

Awesome stuff. Those are actually pretty major bugs the most pressing one being the assign function with const overload. That shouldn't be. There really isn't much test coverage for the TensorMap. Also we don't test Fastor on const/cv-qualified data. So maybe we can enhance that.

What I would be interested in beyond "unit-testing" is application based testing to test Fastor on a full-fledged application. Fastor gets tested as part of other software using Fastor but we don't do it in the master branch here. There are a couple examples of these in the benchmark folder material_benchmark for instance but we don't test that regularly. So feel free to submit a PR for your usecase if you fancy.

I will let this go in for now as it fixes existing bugs. If you have further changes we can always submit later.

All in all, great catch and great work. Thanks.

@romeric romeric merged commit 86d1c14 into romeric:master May 30, 2020
@feltech feltech deleted the bug/assign_from_const_map branch June 2, 2020 21:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants